fix(terminal): ignore errors when killing PTY on Windows#59
Merged
debuglebowski merged 1 commit intodebuglebowski:mainfrom Apr 6, 2026
Merged
Conversation
Wrap session.pty.kill('SIGKILL') in a try/catch so killing an already-exited PTY is a no-op. This prevents the IPC
call from rejecting and allows updateTaskAndNotify to complete (DB update + parent notification), fixing the
working-directory two-click symptom. Typecheck passed.
Co-authored-by: Copilot
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1350
to
+1352
| } catch { | ||
| // Process already exited — not an error | ||
| } |
Contributor
There was a problem hiding this comment.
The analogous resizePty try/catch (around line 1296) records a pty.resize_failed diagnostic event before swallowing the error, which makes it possible to correlate Windows kill failures in diagnostic logs. The new catch here is completely silent.
Consider recording a warn-level diagnostic event so the condition is visible in telemetry without causing the call to fail:
Suggested change
| } catch { | |
| // Process already exited — not an error | |
| } | |
| } catch (error) { | |
| // Process already exited — not an error on Windows | |
| recordDiagnosticEvent({ | |
| level: 'warn', | |
| source: 'pty', | |
| event: 'pty.kill_failed', | |
| sessionId, | |
| taskId: session.taskId, | |
| message: (error as Error).message | |
| }) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/domains/terminal/src/main/pty-manager.ts
Line: 1350-1352
Comment:
**Silent catch reduces observability**
The analogous `resizePty` try/catch (around line 1296) records a `pty.resize_failed` diagnostic event before swallowing the error, which makes it possible to correlate Windows kill failures in diagnostic logs. The new catch here is completely silent.
Consider recording a `warn`-level diagnostic event so the condition is visible in telemetry without causing the call to fail:
```suggestion
} catch (error) {
// Process already exited — not an error on Windows
recordDiagnosticEvent({
level: 'warn',
source: 'pty',
event: 'pty.kill_failed',
sessionId,
taskId: session.taskId,
message: (error as Error).message
})
}
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In Windows, when a task was opened, changing the working directory from the settings would not work. The terminal was not updated and the working directory was still showing the original value. You had to repeat the action a 2nd time to make the change effective.
Solution
Wrap session.pty.kill('SIGKILL') in a try/catch so killing an already-exited PTY is a no-op. This prevents the IPC call from rejecting and allows updateTaskAndNotify to complete (DB update + parent notification), fixing the working-directory two-click symptom. Typecheck passed.
Co-authored-by: Copilot
Greptile Summary
This PR fixes a Windows-specific bug in
killPtywhere callingsession.pty.kill('SIGKILL')on an already-exited PTY process throws an exception, causing the IPC call to reject and preventingupdateTaskAndNotifyfrom completing — manifesting as the "two-click" symptom when changing a task's working directory.Changes:
session.pty.kill('SIGKILL')in atry/catchinsidekillPty(pty-manager.ts) so a throw from an already-dead process is treated as a no-op.sessions.delete+notifySessionChange) is correctly ordered before the kill call, sokillPtystill returnstrueand IPC resolves cleanly even when the catch fires.resizePtytry/catch which records apty.resize_faileddiagnostic event, the new catch block is completely silent, which slightly reduces observability on Windows when this path is hit.Confidence Score: 5/5
Safe to merge — minimal, well-scoped fix with correct ordering of session cleanup before the guarded kill call.
Single-function change with one added try/catch; the session is already removed from the map before
kill()is called, so the return value and all callers are unaffected regardless of whether the catch fires. The pattern is consistent with the existingresizePtyguard in the same file. Only concern is the silent catch vs the diagnostic-logging pattern used elsewhere, which is a style suggestion, not a correctness issue.No files require special attention.
Important Files Changed
session.pty.kill('SIGKILL')in a try/catch to silently swallow the error thrown on Windows when the PTY process has already exited; fix is correct and the session cleanup path (delete + notify) already happens before the kill call, soreturn trueis unaffected.Sequence Diagram
sequenceDiagram participant Renderer participant IPC as IPC Handler (handlers.ts) participant PM as pty-manager.ts (killPty) participant PTY as node-pty Renderer->>IPC: terminal:kill-pty (sessionId) IPC->>PM: killPty(sessionId) PM->>PM: sessions.delete(sessionId) PM->>PM: notifySessionChange() PM->>PTY: session.pty.kill('SIGKILL') alt PTY still alive (Linux/macOS or live Windows process) PTY-->>PM: (success) else PTY already exited (Windows) PTY-->>PM: throws Error PM->>PM: catch — treated as no-op end PM-->>IPC: return true IPC-->>Renderer: true (IPC resolves)Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(terminal): ignore errors when killin..." | Re-trigger Greptile
(4/5) You can add custom instructions or style guidelines for the agent here!